KNOX-2412 - Add Logout Link to Home Page for Select Authentication Pr… by lmccay · Pull Request #372 · apache/knox

lmccay · 2020-09-04T00:40:26Z

What changes were proposed in this pull request?

Logout Button on Homepage
Homepage Logout Configurable from gateway-site.xml
KNOXSSOUT service changed to be able to be hosted in arbitrary topologies - may break existing use of this service in a dedicated knoxssout.xml topology which was the original intent but it was never really documented as supported.
KNOX-SESSION API added to return authenticated user and optionally logoutUrl
A SessionInformation section added to the Homepage with a Welcome {username} and logout button
KNOXSSOUT and KNOX-SESSION added to homepage.xml topology by default
homePageLogoutEnabled defaults to true since the default KnoxSSO with form based IDP is supported.

Planned for Future

Move session-information module into the header
Integrate KNOXSSO with TokenStateServer feature for token revocation for cookie tokens
Extend KNOXSSOUT to revoke tokens on logout
Pluggable logout providers rather than a single logoutUrl mechanism so other IDP logout mechanisms can be integrated

How was this patch tested?

Existing unit tests run
SessionResourceTest unit test added

The following has homePageLogoutEnabled=false

…oviders Change-Id: I963d526cc560d0e2e00745c9b42c7ad63ed29450

smolnar82

It looks really good; I only have some minor comments and questions to address.

smolnar82 · 2020-09-04T06:56:44Z

        <dependency>
            <groupId>org.apache.knox</groupId>
-            <artifactId>gateway-service-metadata</artifactId>
+            <artifactId>gateway-service-session</artifactId>


Why not add the new service as a separate dependency?

Don't know what you mean here.

Not sure why the patch only shows up like this but the original gateway-service-metadata is still in there. I had originally copy and pasted it but forgot to change the name then went back and changed it. The merged file looks fine.

smolnar82 · 2020-09-04T06:59:03Z

  private static final String DEFAULT_SSO_COOKIE_NAME = "hadoop-jwt";

-  static final String RESOURCE_PATH = "/api/v1/webssout";
+  static final String RESOURCE_PATH = "knoxssout/api/v1/webssout";


I don't get why do we need the knoxssout. This change makes the SSOUT API inconsistent with the rest of the APIs.

Not all of the other APIs do that. It was done that way for APIs that were initially intended to be deployed to a single dedicated topology. Like the admin.xml.

smolnar82 · 2020-09-04T07:02:26Z

+import org.apache.knox.gateway.security.SubjectUtils;
+
+@Singleton
+@Path("session/api/v1/")


Perhaps changing this to api/v1/session to be consistent with another API endpoints?

I don't believe so - this is the session API, for now there is a sessioninfo but we could potentially add other things to it. It can be added to any topology and addressed via the session context path.

smolnar82 · 2020-09-04T07:07:44Z

+        console.debug('SessionInformationComponent --> getUser() --> dr.who');
+        return 'dr.who';


Why not returning null?

dr.who is common in Hadoop world. While I don't like it - it is better than the UI saying "Welcome null". :)

smolnar82 · 2020-09-04T07:10:00Z

+        // window.alert('Are you sure???');
+        console.debug('SessionInformationComponent --> attempting logout() --> ');
+        if (this.sessionInformation) {
+            if (this.getLogoutUrl() == null) {


I'd use this.logoutSupported so that the logic if logout is supported happens in one place only (in setSessionInformation).

Good point. I'll make that change - thanks!

smolnar82 · 2020-09-04T07:15:12Z

+        console.debug('SessionInformationComponent --> attempting logout() --> ');
+        if (this.sessionInformation) {
+            if (this.getLogoutUrl() == null) {
+                window.alert('Logout for the configured is IDP not supported.\nPlease close all browser windows to logout.');


There is a more elegant way of notifying the end-users: using a module called sweetalert (https://www.npmjs.com/package/sweetalert). It's been used both on the Admin UI and on the Home Page. See this usage for instance:
https://github.com/apache/knox/blob/master/knox-homepage-ui/home/app/homepage.service.ts#L81

This isn't being used now since it doesn't render the button at all for now. I'm not sure which is better which is why I left it here for now. A missing logout button may seem like a bug but a logout button that doesn't log you out may be bad as well. At least it is known to be intentional and provides some insights though. Planned to follow up with discussion on this. If we go that route we can use sweetalert instead for sure.

Change-Id: If29679793a5ae4629b1f3788459baf5682fe8c35

…tication Pr… (apache#372) * KNOX-2412 - Add Logout Link to Home Page for Select Authentication Providers Change-Id: If7a3acf3a9094b2aa528de9aa1edf3df86b9ef30

* changes: CDPD-14138 KNOX-2408 - Improved AliasBasedTokenState service and house-keeping (apache#371) CDPD-14898 KNOX-2412 - Add Logout Link to Home Page for Select Authentication Pr… (apache#372) CDPD-14138 KNOX-2402 - Adding Gateway performance testing (apache#365)

KNOX-2412 - Add Logout Link to Home Page for Select Authentication Pr…

0b75557

…oviders Change-Id: I963d526cc560d0e2e00745c9b42c7ad63ed29450

lmccay requested review from moresandeep, pzampino and smolnar82 September 4, 2020 00:41

smolnar82 requested changes Sep 4, 2020

View reviewed changes

KNOX-2412 - address review comments

7da5658

Change-Id: If29679793a5ae4629b1f3788459baf5682fe8c35

lmccay requested a review from smolnar82 September 4, 2020 22:50

lmccay merged commit 235bfd4 into apache:master Sep 6, 2020

		console.debug('SessionInformationComponent --> getUser() --> dr.who');
		return 'dr.who';

Conversation

lmccay commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Planned for Future

How was this patch tested?

Uh oh!

smolnar82 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lmccay commented Sep 4, 2020 •

edited

Loading